Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve test_dylink_tls. NFC #14982

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Improve test_dylink_tls. NFC #14982

merged 1 commit into from
Sep 1, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 31, 2021

Make TLS variables static which fixes a linker failure when
building with -g (for example this test currently fails under wasm2g
and wasm0g congigurations).

Also, add two different TLS variables in each of the modules which
helps with debugging since only one of them will be at TLS offset 0.

@sbc100 sbc100 requested review from dschuff and kripken August 31, 2021 19:45
@sbc100
Improve test_dylink_tls. NFC
7d04d82 
Make TLS variables static which fixes a linker failure when
building with `-g` (for example this test currently fails under wasm2g
and wasm0g congigurations).

Also, add two different TLS variables in each of the modules which
helps with debugging since only one of them will be at TLS offset 0.
@dschuff
Copy link
Member

dschuff commented Aug 31, 2021

So I guess this doesn't actually test import and export of TLS, just that TLS variables work in main and side modules?

@dschuff
Copy link
Member

dschuff commented Aug 31, 2021

wait but why does the link fail with the old version?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 31, 2021

So I guess this doesn't actually test import and export of TLS, just that TLS variables work in main and side modules?

Yes exactly, there is a TODO already in place to add TLS import/export testing.

@sbc100 sbc100 enabled auto-merge (squash) August 31, 2021 23:13
@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 31, 2021

wait but why does the link fail with the old version?

This test currently fails with following error when building with -g:

wasm-ld: error: TLS symbols cannot yet be exported: `bar`
emcc: error: '/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/wasm-ld -o side.wasm --whole-archive /tmp/emscripten_temp_gz91gwpy/side_0.o -L/usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/pic /usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/pic/crtbegin.o --no-whole-archive -mllvm -combiner-global-alias-analysis=false -mllvm -disable-lsr --import-undefined --import-memory --shared-memory --export-all --no-gc-sections --experimental-pic -shared' failed (returned 1)

I guess because bar is non-static symbol and we are linking with --export-all.

Its not clear to me why this doesn't also fail without -g but I think this change it worth landed given that it fixes a known failure. I'm currently working TLS import/export so I can investigate and I will investigate and fix that issue.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 31, 2021

wait but why does the link fail with the old version?

This test currently fails with following error when building with -g:

wasm-ld: error: TLS symbols cannot yet be exported: `bar`
emcc: error: '/usr/local/google/home/sbc/dev/wasm/llvm-build/bin/wasm-ld -o side.wasm --whole-archive /tmp/emscripten_temp_gz91gwpy/side_0.o -L/usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/pic /usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/pic/crtbegin.o --no-whole-archive -mllvm -combiner-global-alias-analysis=false -mllvm -disable-lsr --import-undefined --import-memory --shared-memory --export-all --no-gc-sections --experimental-pic -shared' failed (returned 1)

I guess because bar is non-static symbol and we are linking with --export-all.

Its not clear to me why this doesn't also fail without -g but I think this change it worth landed given that it fixes a known failure. I'm currently working TLS import/export so I can investigate and I will investigate and fix that issue.

I figured it out. The reason is that we have relocations of type R_WASM_MEMORY_ADDR_I32 in the debug info section which forces the address to be imported as GOT.mem.bar whereas the code section we generate __tls_base+R_WASM_MEMORY_ADDR_TLS_SLEB.

I guess on native platforms you would end up with dynamic relocations in the debug info section? But we currently don't support that. Even with TLS imports which I'm hoping to land this week I don't we can change the debug info sections to use global.get to retrieve addresses.. so I guess we need to find some kind of dyanmic relocation solution for DWARF?

dschuff
dschuff approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this change is fine. I just wanted to make sure we understood what's happening here so we pick it up when we're ready.

@sbc100 sbc100 merged commit 4e95fbb into main Sep 1, 2021
@sbc100 sbc100 deleted the improve_tls_test branch September 1, 2021 00:17
@dschuff
Copy link
Member

dschuff commented Sep 1, 2021

I guess on native platforms you would end up with dynamic relocations in the debug info section? But we currently don't support that. Even with TLS imports which I'm hoping to land this week I don't we can change the debug info sections to use global.get to retrieve addresses.. so I guess we need to find some kind of dyanmic relocation solution for DWARF?

Ick :(

This was referenced Jan 6, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschuff dschuff dschuff approved these changes

@kripken kripken Awaiting requested review from kripken

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @dschuff